[MNG-6357] Dependency order should be nearest first by belingueres · Pull Request #281 · apache/maven

belingueres · 2019-08-31T15:39:58Z

Save the artifacts temporarily in a Map ordered by tree depth, and only
after finishing iterating over the dependency tree are the artifacts
added to the collection in depth order.

Note: IT-core test needs to be added replacing IT 3813 which does not apply any longer for a maven version containing this change.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MNG-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
[] You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

belingueres · 2019-08-31T15:42:46Z

Please review this patch. Tested it and seems to solve:

MCOMPILER-317
Modulepath lists dependencies in wrong order mojohaus/exec-maven-plugin#91

rfscholte · 2019-08-31T15:53:44Z

Looks good at first sight. This code deserves a unittest, that's better than relying on plugins. I don't think the unittest is too hard to write.
I noticed you're not using diamond operators. Looking at the code it should be written with the new Map methods, but that requires Java 8. The moment that Maven will require Java 8 is getting closer

michael-o · 2019-08-31T18:16:15Z

@rfscholte We have more serious problems that introducing fancy Java 8 features.

Tibor17 · 2019-08-31T18:35:33Z

                                    DependencyFilter filter )
+    {
+        Map<Integer, List<org.apache.maven.artifact.Artifact>> artifactsByDepth =
+            new TreeMap<Integer, List<org.apache.maven.artifact.Artifact>>();


you can use diamond <>. It is Java 1.7 feature.

If i open IDEA, it shows me these issues related to Java version.

Tibor17 · 2019-08-31T18:36:32Z

+        Map<Integer, List<org.apache.maven.artifact.Artifact>> artifactsByDepth =
+            new TreeMap<Integer, List<org.apache.maven.artifact.Artifact>>();
+
+        ArrayList<org.apache.maven.artifact.Artifact> firstLevelArtifacts =


aaa, Collection or List better than ArrayList.

Tibor17 · 2019-08-31T18:37:06Z

+            new TreeMap<Integer, List<org.apache.maven.artifact.Artifact>>();
+
+        ArrayList<org.apache.maven.artifact.Artifact> firstLevelArtifacts =
+            new ArrayList<org.apache.maven.artifact.Artifact>( nodes.size() );


Tibor17 · 2019-08-31T18:37:25Z

+                List<org.apache.maven.artifact.Artifact> artifactsCurrentDepth = artifactsByDepth.get( currentDepth );
+                if ( artifactsCurrentDepth == null )
+                {
+                    artifactsCurrentDepth = new ArrayList<org.apache.maven.artifact.Artifact>();


jmhodges · 2021-12-29T05:08:08Z

The merge conflict is pretty straight-forward to fix (it's a simple collision in the pom.xml). Would love to see this go in! Happy to provide a new PR if that's preferable

Save the artifacts temporarily in a Map ordered by tree depth, and only after finishing iterating over the dependency tree are the artifacts added to the collection in depth order.

- used diamond <> operator. - changed ArrayList for List in variable declaration. - Added unit test.

- Re added maven-resolver-test-util as a test dependency. - Changed tests to JUnit 5.

cowwoc · 2023-02-18T18:32:03Z

Per https://maven.apache.org/download.cgi Maven 4.x requires JDK 8. What is needed to move this PR forward?

jira-importer · 2025-07-06T09:07:49Z

Resolve #7501

gnodet

AI Code Review — PR #281

Verdict: ⚠️ Needs rebase

This PR changes RepositoryUtils.toArtifacts() to produce artifacts ordered by dependency tree depth (breadth-first / nearest-first) instead of depth-first. The approach using a TreeMap<Integer, List<Artifact>> keyed by depth is logically correct and addresses a legitimate issue (MNG-6357).

However, the PR is 4+ years stale and cannot merge.

Findings

#	Severity	File	Details
1	🔴 High	`RepositoryUtils.java`	Stale path — File moved to `impl/maven-core/...` by MNG-8346. Merge state is CONFLICTING.
2	🔴 High	`RepositoryUtilsTest.java`	Test file conflict — A `RepositoryUtilsTest.java` already exists on master (added by commit `25c80d8ece`). The PR's test class would need to be merged into the existing test file. Also, `@author` tags were removed from the codebase (commit `0b3246381d`).

Behavioral concern

This change affects every caller of RepositoryUtils.toArtifacts() — including plugin classpath construction and project artifact resolution. While breadth-first ordering is arguably more correct, any code that implicitly depends on the current depth-first ordering could break. The PR description acknowledges that IT-3813 "does not apply any longer," suggesting awareness of this risk. A full integration test run is essential after rebasing.

Recommendation

If MNG-6357 is still desired: (1) rebase onto current master, (2) merge tests into the existing RepositoryUtilsTest.java, (3) remove @author tag, (4) run the full integration test suite to validate no regressions.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

Reviewed 3 PRs: apache#144 (COMMENT), apache#1125 (COMMENT), apache#281 (COMMENT). 36 total reviews posted. ~23 PRs remaining. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

belingueres · 2026-07-15T02:13:19Z

Let me know if this patch is still needed to do the recommended changes.

Tibor17 reviewed Aug 31, 2019

View reviewed changes

jmhodges mentioned this pull request Dec 29, 2021

[MNG-6357] Dependency order should be nearest first apache/maven-integration-testing#47

Closed

belingueres added 3 commits December 30, 2021 18:02

[MNG-6357] Dependency order should be nearest first

d2df778

Save the artifacts temporarily in a Map ordered by tree depth, and only after finishing iterating over the dependency tree are the artifacts added to the collection in depth order.

Changes based on review:

12fe6fc

- used diamond <> operator. - changed ArrayList for List in variable declaration. - Added unit test.

Modifications after rebase

2f29c36

- Re added maven-resolver-test-util as a test dependency. - Changed tests to JUnit 5.

belingueres force-pushed the MNG-6357 branch from ab9deb4 to 2f29c36 Compare January 3, 2022 21:40

hboutemy force-pushed the master branch from a075afa to 6e50886 Compare March 24, 2024 21:16

jira-importer mentioned this pull request Feb 23, 2025

[MNG-6357] Dependency order should be nearest first #7501

Open

gnodet reviewed Jul 10, 2026

View reviewed changes

Uh oh!

Conversation

belingueres commented Aug 31, 2019

Uh oh!

belingueres commented Aug 31, 2019

Uh oh!

rfscholte commented Aug 31, 2019

Uh oh!

michael-o commented Aug 31, 2019

Uh oh!

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

jmhodges commented Dec 29, 2021

Uh oh!

cowwoc commented Feb 18, 2023

Uh oh!

jira-importer commented Jul 6, 2025

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

AI Code Review — PR #281

Findings

Behavioral concern

Recommendation

Uh oh!

belingueres commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants